Conversation
| self._executor_service = futures.ThreadPoolExecutor(max_workers=max_workers) | ||
| self._cancelledRequests = set() | ||
|
|
||
| def init_async(self): |
There was a problem hiding this comment.
Shouldn't we have a way to set a asyncio event loop executor by parameter? https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.set_event_loop
There was a problem hiding this comment.
This init_async function was more a relict of a previous attempt... I think it is save to initialize the queue in the default initializer...
Regarding set_event_loop I don't how I achieve something similar to davschul/python-lsp-server@215f79d in a more ellegant way, And I'm open to suggestions :)
There was a problem hiding this comment.
No worries, this can be looked in a future iteration.
pylsp_jsonrpc/endpoint.py
Outdated
| async def consume_task(self): | ||
| log.warning("starting task") | ||
| loop = asyncio.get_running_loop() | ||
| while True: |
There was a problem hiding this comment.
Is this infinite loop being exited at any point? i.e., what happens at teardown time
There was a problem hiding this comment.
I guess the loop is never cleanly exited, let me check what python has to offer to stop this loop here.
|
@davschul, thanks for the changes, we would appreciate if you added some tests for the async interface |
The last changeset addresses the shutdown, I'll have another look for the tests next |
Took a first stab at adding tests |
|
|
||
| def init_async(self): | ||
| self._messageQueue = asyncio.Queue() | ||
| self._consume_task = asyncio.create_task(self.consume_task()) |
There was a problem hiding this comment.
Will this task run on the executor?
There was a problem hiding this comment.
This task will not run in an executor from https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor, but needs to be started after the event loop was started. I pushed an example implementation to davschul/python-lsp-server@9f0f03d
There was a problem hiding this comment.
The infinite loop has potential to block the main event loop, which could lead to performance degradation/unexpected blockage. I think in this case it would be better to run this on a separate event loop
There was a problem hiding this comment.
The asyncio queues used to send the messages cannot be used from different event loops. What might cause the block here in your opinion?
There was a problem hiding this comment.
Sorry for the long break, I was side tracked a bit in the python lsp support in Qt Creator and some urgent bugs before our release. I added a restriction to the loop, but I guess that not really what you've expected right?
|
Sorry for forgetting about this one! I think it looks better than a |
|
I don't know why the tests are not running |
|
Closing and reopening to see if they run. |
|
Sorry to bother @andfoy and @ccordoba12 ; has this PR been forgotten? It seems somewhat helpful. |
|
No, it hasn't. The thing is we also need to make |
4 participants
This is a very early version of async message reading with a request for comments whether this in general would be acceptable and what I need to do to make sure this doesn't break anything.